Skip to content

feat: Support for recursive and circular references using lazy imports #670

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 32 commits into from
Nov 12, 2022
Merged

feat: Support for recursive and circular references using lazy imports #670

merged 32 commits into from
Nov 12, 2022

Conversation

mtovt
Copy link
Contributor

@mtovt mtovt commented Sep 13, 2022 •
edited
Loading

Continuation of #582. Seems, closes: #338, #466 Adds lazy imports to avoid circular import errors, which #582 has.
Not fully finished. I’d like to polish it after receiving feedback.

Instead of #636

todo:

  • Remove useless pass, which sometimes appears in to_dict method of model.
  • Make type checking of generated client pass (Seems a type in quotes without import does not satisfy Mypy. Seems may be fixed by using TypeVar)
  • Add test related to quoted argument if this approach will be used.
  • Do not use lazy imports if a model does not spawn circular import error (I’d say it separate PR)

maz808 and others added 17 commits January 29, 2022 22:06
@maz808
Add support for recursive/circular refs
174f090 
This commit adds support for recursive and circular references in object
properties and additionalProperties, but not in allOf. The changes
include:

-Delayed processing of schema model properties
-Cascading removal of invalid schema reference dependencies
-Prevention of self import in ModelProperty relative imports
-Prevention of forward recursive type reference errors in generated modules
-Logging for detection of recursive references in allOf
@maz808
Fix array schema object item issues
d9cb65d 
Previous changes prevented models defined in array schemas from being
built properly. This commit fixes that issue and adds support for
recursive and circular references in array schema item objects, a
behavior that was previously expected but not functioning correctly. This
does not add support for recursive and circular references directly in an
array schema's 'items' section. However, this feature would be
functionally useless, so a warning is logged on detection.
@dbanty
Merge branch 'main' into recursive-references
43c58d1
@mtovt
Merge branch 'main' into pr/582
2512e5d
@mtovt
Merge branch 'main' into pr/582
98b3d79
@mtovt
try to add lazy imports to avoid circular import issue
f3eaf74
@mtovt
Fix tests related to the addition of lazy imports
d384dcb
@mtovt
Merge remote-tracking branch 'upstream/main' into pr/582
7cce9e7
@mtovt
Fix class name
e11f9cc
@mtovt
fix: quoting properties type correctly
1bcf7bb
@mtovt
Add lazy import to the Union and List properties
b407d52
@mtovt
Fix: add missing lazy imports to the additional properties. Make mode…
ca22a3a 
…l quoting optional
@mtovt
Fix: remove extra lazy imports in list property
8e6a9dc
@mtovt
Add lazy imports to the relatives in Endpoints
d8c60ef
@mtovt
Use quoted arg and base type checking. Enum properties are not quot…
2524509 
…ing now.
@mtovt
Revert changed test
93ee14d
@mtovt
Update e2e: models
cadad67
@mtovt mtovt changed the title Feature/lazy imports feat: Support for recursive and circular references using lazy imports Sep 13, 2022
@codecov
Copy link

codecov bot commented Sep 18, 2022 •
edited
Loading

Codecov Report

Merging #670 (42a56da) into main (47e576c) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              main      #670    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           49        49            
  Lines         1800      1946   +146     
==========================================
+ Hits          1800      1946   +146
Impacted Files Coverage Δ
openapi_python_client/parser/openapi.py 100.00% <100.00%> (ø)
...penapi_python_client/parser/properties/__init__.py 100.00% <100.00%> (ø)
...i_python_client/parser/properties/enum_property.py 100.00% <100.00%> (ø)
..._python_client/parser/properties/model_property.py 100.00% <100.00%> (ø)
...penapi_python_client/parser/properties/property.py 100.00% <100.00%> (ø)
openapi_python_client/parser/properties/schemas.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@dbanty
Copy link
Collaborator

dbanty commented Sep 18, 2022

Thanks for continuing this journey—this is the most challenging problem that's been solved for this project in a long time 😁. This approach looks good—it's complex, but this is the cleanest solution I've seen so far, and I think about as clean as it will ever get. Well done!

I solved the mypy issue, and I think that checks are passing now (though GitHub is acting weird for me).

🥳 🎉 🙇

This was referenced Sep 18, 2022
Closed
Closed
Closed
@dbanty dbanty added this to the 0.12.0 milestone Sep 18, 2022
@dbanty dbanty added the ✨ enhancement New feature or improvement label Sep 18, 2022
@dbanty
Copy link
Collaborator

dbanty commented Sep 27, 2022

I figured out where that pass is coming from but haven't fixed it yet. All of the types used in annotations are imported again, but some of them are not required to convert the type to a dict (e.g., ModelWithCircularRefInAdditionalPropertiesA). In this case, when autoflake runs and deletes unused imports, it leaves a pass statement when all imports were deleted (for some reason).

Not sure how to solve this without adding more complexity to selecting imports.

@mtovt
Copy link
Contributor Author

mtovt commented Oct 2, 2022

@dbanty Thank you so much for the fast feedback and your help!

Seems all required tests were added.

autoflake currently depends on temporarily replacing unused imports/variables with pass and then removing the useless ones in a second pass.

Seems autoflake has had this issue for several years.
PyCQA/autoflake#52
PyCQA/autoflake#65

I suggest fixing it later since it doesn’t produce any troubles + there are several points to improve and may be better to do it centralized.

Checks now are failing because of poetry export -f requirements.txt | poetry run safety check --bare --stdin\ command. Have not investigated yet.

@dbanty dbanty removed the ✨ enhancement New feature or improvement label Oct 2, 2022
@dbanty
Copy link
Collaborator

dbanty commented Oct 2, 2022

Looks like the locked version of pydantic has a vulnerability, poetry update pydantic should fix it. I need to find out why Renovate isn't auto updating the lock file anymore 🤔

@mtovt
Copy link
Contributor Author

mtovt commented Oct 17, 2022

Hello! Seems the review is required here to be merged. I didn't see how to request it explicitly. Just let me know if I should do something here.

@dbanty
Copy link
Collaborator

dbanty commented Oct 17, 2022

Yup, I think this is good to go, I’ve just been traveling pretty much nonstop since the last update 😅. This will make it into the next release though! 🎉

@dbanty dbanty merged commit 8300674 into openapi-generators:main Nov 12, 2022
@dbanty dbanty mentioned this pull request Nov 12, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.12.0
Development

Successfully merging this pull request may close these issues.

Add support for recursively defined schemas
3 participants
@mtovt @dbanty @maz808